node:url: run domainToUnicode regardless of string backing store by robobun · Pull Request #33756 · oven-sh/bun

robobun · 2026-07-08T12:39:58Z

url.domainToUnicode returned its argument verbatim whenever the input WTF::String happened to be 16-bit backed, so the documented round trip never decoded and two === strings could produce different outputs.

Repro

import { domainToASCII, domainToUnicode } from "node:url";

domainToUnicode(domainToASCII("bücher.de"));
// bun: "xn--bcher-kva.de"   node: "bücher.de"

domainToUnicode("中.xn--fiqs8s");
// bun: "中.xn--fiqs8s"       node: "中.中国"

const a = "xn--bcher-kva.de";
const b = "xn--bcher-kva.de中".slice(0, -1);   // === a, 16-bit backed
console.log(a === b, domainToUnicode(a), domainToUnicode(b));
// bun: true "bücher.de" "xn--bcher-kva.de"
// node: true "bücher.de" "bücher.de"

Cause

jsDomainToUnicode in src/jsc/bindings/NodeURL.cpp short-circuited on !domain.is8Bit():

if (!domain.is8Bit())
    // this function is only for undoing punycode so its okay if utf-16 text makes it out unchanged.
    return JSC::JSValue::encode(arg0);

is8Bit() is a representation property of WTF::String, not a value property. An all-ASCII punycode host can be stored in a 16-bit buffer: domainToASCII always returns one (it builds the result from a char16_t[]), and so do slices of any 16-bit rope and literals from a source file containing non-Latin-1 text. A 16-bit string can also contain xn-- A-labels mixed with already-decoded labels ("中.xn--fiqs8s"), and UTS #46 ToUnicode is defined over all inputs, including mapping and case folding of code points above U+00FF.

Fix

Drop the fast path and always call uidna_nameToUnicode, matching the structure jsDomainToASCII already uses for the 8-bit/16-bit split.

Verification

test/js/node/url/url-domain-ascii-unicode.test.js: 134 pass. The four new tests fail on released Bun and pass with this change; the 130 pre-existing tests are unchanged.

#33206 is a broader rewrite of both domainToASCII and domainToUnicode through the WHATWG host parser that also removes this fast path; this PR is the minimal targeted fix for the representation-dependent result and conflicts only on the three lines it deletes.

jsDomainToUnicode returned its argument verbatim whenever the input WTF::String was 16-bit backed. is8Bit() is a representation property, not a value property: an all-ASCII punycode label can be stored in a 16-bit buffer (domainToASCII always returns one, as do slices of a 16-bit rope), and UTS #46 ToUnicode is defined over all inputs, not only Latin-1 ones. The result was that domainToUnicode(domainToASCII(d)) never decoded, and two === strings could produce different outputs. Drop the fast path and always call uidna_nameToUnicode, matching the structure jsDomainToASCII already uses.

robobun · 2026-07-08T12:40:02Z

Reproduced with:

bun -e 'import {domainToASCII as a, domainToUnicode as u} from "node:url"; const x="xn--bcher-kva.de", y=(x+"中").slice(0,-1); console.log(x===y, u(x), u(y), u(a("bücher.de")))'

Fix in #33756.

CI: url-domain-ascii-unicode.test.js passes on every lane. The remaining red is unrelated infra flake (build 70489: postgres ECONNREFUSED on win-x64; build 70495: MySQL container health-check timeout on musl-aarch64 for 26030.test.ts). Ready for review.

robobun · 2026-07-08T12:40:08Z

^{Updated 9:38 AM PT - Jul 8th, 2026}

❌ @robobun, your commit 7ca8fa5 has 3 failures in Build #70495 (All Failures):

test/integration/next-pages/test/dev-server.test.ts - code 1 on 🍎 14 aarch64
test/js/bun/terminal/terminal.test.ts - code 1 on 🍎 14 x64
test/regression/issue/26030.test.ts - code 1 on 🐧 3.23 aarch64
test/regression/issue/26030.test.ts - code 1 on 🐧 3.23 x64

🧪 To try this PR locally:

bunx bun-pr 33756

That installs a local version of the PR into your bun-33756 executable, so you can run:

bun-33756 --bun

coderabbitai · 2026-07-08T12:42:04Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ef7a83c7-13bb-4ba1-81fe-031d8eb61f5c

📥 Commits

Reviewing files that changed from the base of the PR and between 332f744 and 7ca8fa5.

📒 Files selected for processing (2)

src/jsc/bindings/NodeURL.cpp
test/js/node/url/url-domain-ascii-unicode.test.js

Walkthrough

This PR changes jsDomainToUnicode so non-8-bit inputs proceed through IDNA conversion instead of being returned unchanged. It also adds tests for string-representation stability, round-tripping, mixed-label A-label decoding, and UTS #46 mapping.

Changes

domainToUnicode IDNA Conversion Fix

Layer / File(s)	Summary
IDNA conversion path `src/jsc/bindings/NodeURL.cpp`	Removes the non-8-bit early return in `jsDomainToUnicode`, converts only 8-bit inputs to UTF-16, and allows all inputs to reach `uidna_nameToUnicode`.
domainToUnicode test coverage `test/js/node/url/url-domain-ascii-unicode.test.js`	Adds assertions for equal-string representation stability, Unicode-to-ASCII-to-Unicode round-tripping, A-label decoding with mixed labels, and UTS `#46` mapping.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: always decoding domainToUnicode independent of string backing store.
Description check	✅ Passed	The description covers what changed, why, a repro, the fix, and verification, though it uses different headings than the template.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{Comment @coderabbitai help to get the list of available commands.}

claude

LGTM — removes a demonstrably incorrect fast path and mirrors the existing jsDomainToASCII structure, with tests covering the representation-dependence, round-trip, and mixed-label cases.

Extended reasoning...

Overview

This PR touches two files: a 3-line functional change in src/jsc/bindings/NodeURL.cpp (removing the !domain.is8Bit() early-return in jsDomainToUnicode and replacing it with the same if (is8Bit()) convertTo16Bit() pattern used by jsDomainToASCII directly above), and four new test cases appended to test/js/node/url/url-domain-ascii-unicode.test.js.

The root cause is well-argued and correct: WTF::String::is8Bit() reflects the backing store, not the value, so the old fast path made domainToUnicode return different results for ===-equal strings and broke the documented domainToUnicode(domainToASCII(x)) round trip (since domainToASCII builds its result from a char16_t[]). The fix unconditionally routes through uidna_nameToUnicode, which is what UTS #46 ToUnicode requires anyway.

Security risks

None. This is a Node-compat IDNA decoding helper; the change removes a short-circuit and hands the same input to the same ICU function that already handles the 8-bit case. No new allocation sizing, no new parsing of untrusted lengths, no auth/crypto/permissions surface.

Level of scrutiny

Low. The diff is tiny, mechanical, and mirrors the sibling function line-for-line. The new code path (span16() → uidna_nameToUnicode) is the exact path already exercised by every 8-bit input after convertTo16Bit(), so there's no genuinely new behavior being introduced — just the removal of an incorrect bypass.

Other factors

The added tests are strong: they assert the specific representation-independence property (two === strings produce the same output), the round-trip over the existing 50+ pair fixture table, mixed A-label/U-label inputs, and UTS #46 mapping on non-Latin-1 input. The PR description verifies the new tests fail on released Bun and pass with the change, and notes the relationship to the broader #33206 rewrite. No outstanding reviewer comments, no bugs flagged by the bug-hunting pass.

github-actions Bot added the claude label Jul 8, 2026

claude Bot reviewed Jul 8, 2026

View reviewed changes

ci: retrigger

7ca8fa5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

node:url: run domainToUnicode regardless of string backing store#33756

node:url: run domainToUnicode regardless of string backing store#33756
robobun wants to merge 2 commits into
mainfrom
farm/fd2aa854/domaintounicode-16bit

robobun commented Jul 8, 2026

Uh oh!

robobun commented Jul 8, 2026 •

edited

Loading

Uh oh!

robobun commented Jul 8, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 8, 2026 •

edited

Loading

Uh oh!

claude Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

robobun commented Jul 8, 2026

Repro

Cause

Fix

Verification

Uh oh!

robobun commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Jul 8, 2026 •

edited

Loading

robobun commented Jul 8, 2026 •

edited

Loading

coderabbitai Bot commented Jul 8, 2026 •

edited

Loading